Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: improve admonitions notes #1549

Merged
merged 2 commits into from
Sep 2, 2024
Merged

feat: improve admonitions notes #1549

merged 2 commits into from
Sep 2, 2024

Conversation

bjohansebas
Copy link
Member

Colors have been changed because their meanings were not visually distinguishable, making the documentation difficult to read. Additionally, in dark mode, the colors were not displayed correctly.

Notice:
image
image
image
image
Note:
image
image
image
image
Warn:
image
image
image
image

@netlify Netlify
Copy link

netlify bot commented Jul 28, 2024 •
edited
Loading

✅ Deploy Preview for expressjscom-preview ready!

Name Link
🔨 Latest commit 5e9bc48
🔍 Latest deploy log https://app.netlify.com/sites/expressjscom-preview/deploys/66aad90cae6e9500086192ea
😎 Deploy Preview https://deploy-preview-1549--expressjscom-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@crandmck
Copy link
Member

Thanks @bjohansebas in general, this looks good.

These things are typically called "admonitions" not "asides". Could you change the directory to that just for sake of conforming to industry standards.

Also, this is a fairly sweeping change and I didn't have time to review it in depth yet. Maybe @chrisdel101 or @inigomarquinez can help on that front.

@chrisdel101
Copy link
Contributor

The code changes look fine to me. I didn't go through every single page in preview but the ones I did see look pretty nice!
@bjohansebas If you want to make those directory changes as requested.

@bjohansebas
Copy link
Member Author

@crandmck @chrisdel101 thanks for the recommendation; the directory name change has been made.

@bjohansebas bjohansebas changed the title feat: improve aside notes feat: improve admonitions notes Aug 1, 2024
@crandmck
Copy link
Member

I'll land this, but would like to understand why the check-translation test is failing.

@bjohansebas
Copy link
Member Author

@crandmck It's an issue we've been referencing in #1553. Probably @IamLizu will have the solution and explanation once it's resolved.

@crandmck crandmck merged commit f24789a into expressjs:gh-pages Sep 2, 2024
6 of 7 checks passed
@bjohansebas bjohansebas deleted the improve-notes branch September 2, 2024 20:31
chrisdel101 pushed a commit to chrisdel101/expressjs.com that referenced this pull request Nov 3, 2024
@bjohansebas
feat: improve admonitions notes (expressjs#1549)
f7f0e68 
* feat: change color of aside notes

* rename aside dir to admonitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlosstenzel carlosstenzel carlosstenzel approved these changes

@chrisdel101 chrisdel101 Awaiting requested review from chrisdel101

@crandmck crandmck Awaiting requested review from crandmck

@inigomarquinez inigomarquinez Awaiting requested review from inigomarquinez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bjohansebas @crandmck @chrisdel101 @carlosstenzel